Fix TLA sibling dynamic-import TDZ regression (#30634)#30639
Conversation
|
Updated 10:59 AM PT - May 14th, 2026
❌ @robobun, your commit 4c57af9 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 30639That installs a local version of the PR into your bun-30639 --bun |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughWebKit build version is bumped to a preview identifier, and test coverage for dynamic-import and top-level-await cycles is refactored: two concrete deadlock tests become placeholders, and a new concurrent-import test validates that sibling imports with TLA and reexports execute safely. ChangesWebKit and TLA Test Updates
Possibly Related Issues
Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build/deps/webkit.ts`:
- Line 6: The WEBKIT_VERSION constant currently points to a non-existent preview
release ("preview-pr-228-3a85cc44") causing 404s when downloading artifacts; fix
by either creating the matching release/tag and uploading the required platform
artifacts in the oven-sh/WebKit repo, or update the WEBKIT_VERSION value to a
known existing release tag (replace the string in the WEBKIT_VERSION export)
that contains the prebuilt artifacts you need; ensure the chosen tag is
reachable and contains assets for all target platforms so the build can download
successfully.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bdb2e52a-2353-47a5-accb-86e79eb8f5b3
📒 Files selected for processing (2)
scripts/build/deps/webkit.tstest/js/bun/resolve/dynamic-import-tla-cycle.test.ts
| * From https://github.com/oven-sh/WebKit releases. | ||
| */ | ||
| export const WEBKIT_VERSION = "5488984d20e0dbfe4be2c3ba8fb18eb81a5e0e8b"; | ||
| export const WEBKIT_VERSION = "preview-pr-228-3a85cc44"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the WebKit preview release and upstream PR exist
# Check if the release tag exists and list its assets
echo "=== Checking release tag ==="
gh api repos/oven-sh/WebKit/releases/tags/autobuild-preview-pr-228-3a85cc44 \
--jq '{name: .name, tag: .tag_name, draft: .draft, prerelease: .prerelease, assets: [.assets[].name]}' \
|| echo "ERROR: Release tag not found"
# Check if PR `#228` exists
echo -e "\n=== Checking upstream PR `#228` ==="
gh api repos/oven-sh/WebKit/pulls/228 \
--jq '{number: .number, title: .title, state: .state, merged: .merged}' \
|| echo "ERROR: PR not found"Repository: oven-sh/bun
Length of output: 445
The release tag autobuild-preview-pr-228-3a85cc44 does not exist in oven-sh/WebKit.
The WEBKIT_VERSION points to a preview release that cannot be found (HTTP 404). When the build attempts to download prebuilt artifacts, it will fail. The upstream PR #228 exists and is open (not yet merged), which explains why the preview release doesn't exist.
Either create the release tag in oven-sh/WebKit with the required platform artifacts, or point WEBKIT_VERSION to an existing stable release and track the upstream PR separately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/build/deps/webkit.ts` at line 6, The WEBKIT_VERSION constant
currently points to a non-existent preview release ("preview-pr-228-3a85cc44")
causing 404s when downloading artifacts; fix by either creating the matching
release/tag and uploading the required platform artifacts in the oven-sh/WebKit
repo, or update the WEBKIT_VERSION value to a known existing release tag
(replace the string in the WEBKIT_VERSION export) that contains the prebuilt
artifacts you need; ensure the chosen tag is reachable and contains assets for
all target platforms so the build can download successfully.
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| * From https://github.com/oven-sh/WebKit releases. | ||
| */ | ||
| export const WEBKIT_VERSION = "5488984d20e0dbfe4be2c3ba8fb18eb81a5e0e8b"; | ||
| export const WEBKIT_VERSION = "preview-pr-228-3a85cc44"; |
There was a problem hiding this comment.
🔴 WEBKIT_VERSION is set to the temporary preview tag preview-pr-228-3a85cc44 instead of a commit SHA. prebuiltUrl() (lines 76-78) only special-cases the autobuild- prefix, so the download URL becomes .../releases/download/autobuild-preview-pr-228-3a85cc44/... and prebuilt-mode builds will 404; it also corrupts process.versions.webkit and the cache key (prebuiltDestDir() slices to preview-pr-228-3). This needs to be replaced with the merged commit hash from oven-sh/WebKit#228 before landing.
Extended reasoning...
What the bug is
WEBKIT_VERSION at scripts/build/deps/webkit.ts:6 is changed from a 40-char commit SHA (5488984d20e0dbfe4be2c3ba8fb18eb81a5e0e8b) to "preview-pr-228-3a85cc44", a temporary preview-build tag for the still-open oven-sh/WebKit#228. The doc comment immediately above (lines 1-5) explicitly documents this constant as a "WebKit commit — determines prebuilt download URL + what to checkout for local mode", and git log -p on this file shows every prior value has been a full commit SHA. Nothing in scripts/ handles a preview-pr- prefix (grep finds no other occurrence).
Code path that triggers it
config.ts:56 wires WEBKIT_VERSION directly into cfg.webkitVersion with no normalization. prebuiltUrl() at webkit.ts:76-78 then computes the release tag as:
const tag = version.startsWith("autobuild-") ? version : `autobuild-${version}`;
return `https://github.com/oven-sh/WebKit/releases/download/${tag}/${name}.tar.gz`;There is no branch for preview-pr-. Since the default cfg.webkit mode is "prebuilt" (used by CI and most local builds), this URL is the one fetched whenever the WebKit dep is resolved.
Why existing code doesn't prevent it
The only special case in prebuiltUrl() is the autobuild- prefix check, intended for users who pass --webkit-version=autobuild-<sha> verbatim. A preview-pr-... value falls into the else branch and gets autobuild- prepended. The override-detection warning at config.ts:1073-1076 ("you're not using the default WebKit version") only fires when the runtime value differs from the default — here the bad value is the default, so no warning fires.
Step-by-step proof
cfg.webkitVersion = "preview-pr-228-3a85cc44"(default fromWEBKIT_VERSION).prebuiltUrl():"preview-pr-228-3a85cc44".startsWith("autobuild-")→false.tag = "autobuild-preview-pr-228-3a85cc44".- URL =
https://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-228-3a85cc44/bun-webkit-linux-amd64-lto.tar.gz. - oven-sh/WebKit publishes preview builds under
preview-pr-<n>-<sha>tags, notautobuild-preview-pr-..., so this asset does not exist → the WebKit fetch step 404s and the build fails. - Even if such a release happened to exist, preview-PR tags are ephemeral and are GC'd once the WebKit PR merges, so any commit on
mainpinned to it would rot.
Secondary impact
depVersionsHeader.ts:59emitscfg.webkitVersionverbatim into the generated header, soprocess.versions.webkitwould report"preview-pr-228-3a85cc44"instead of a commit SHA.prebuiltDestDir()atwebkit.ts:86keys the cache on.slice(0, 16)="preview-pr-228-3", dropping the trailing commit-ish entirely; a future preview of the same PR would collide with this cache entry.- Local mode ("what to checkout") would attempt to check out a ref that isn't a commit in the WebKit repo.
Fix
Replace "preview-pr-228-3a85cc44" with the 40-char commit SHA of the merged oven-sh/WebKit#228 (i.e. wait for that PR to land, then pin to its merge commit) before merging this PR. This is the standard cross-repo workflow placeholder that needs to be swapped before landing.
|
Heads-up for maintainers: this is a WebKit-only fix (oven-sh/WebKit#228) with a `WEBKIT_VERSION` bump here. The internal gate/CI can only verify the fix end-to-end once the WebKit preview build at `autobuild-preview-pr-228-3a85cc44` publishes its tarball (currently in_progress — https://github.com/oven-sh/WebKit/actions/runs/25827484733). Once that lands, Bun CI should link the patched JSC and the new regression test passes. Verified locally with a custom WebKit build: |
|
CI is red on every lane due to the WebKit preview tarball 404 — oven-sh/WebKit#228's build is still in_progress (~18min at last check) at https://github.com/oven-sh/WebKit/actions/runs/25827484733. Once it publishes Needs maintainer to either (a) let WebKit#228 land first and then re-point |
| test.todo("dynamic import inside TLA whose target imports the awaiter back does not deadlock"); | ||
| test.todo("dynamic import inside TLA of a non-entry module whose target imports it back does not deadlock"); |
There was a problem hiding this comment.
🔴 Converting the two reduced self-deadlock tests to .todo is correct, but the real Nitro-output fixture they were reduced from was not updated: test/js/bun/http/bun-server.test.ts:405 ("should be able to parse source map and fetch small stream") spawns js-sink-sourmap-fixture/index.mjs, whose top-level await fetch(/stream) triggers import('./chunks/stream.mjs'), and chunks/stream.mjs:6 statically imports back ../index.mjs. With the 11.c.v skip dropped that self-deadlocks, Bun.serve keeps the loop alive so there's no unsettled-TLA exit, and the test calls Bun.spawnSync with no timeout — so the entire bun-server.test.ts file will hang in CI. That test needs the same .todo treatment (or at minimum a spawnSync timeout) before this lands.
Extended reasoning...
What the bug is
The WebKit change (oven-sh/WebKit#228) drops the Bun-specific deadlock-avoidance skip at innerModuleEvaluation step 11.c.v. The PR description acknowledges that the "Nitro-style await import(./chunk)-loops-back" pattern now matches spec/Node behaviour (deadlock), and accordingly converts the two reduced reproductions in dynamic-import-tla-cycle.test.ts to test.todo.
However, those reduced tests were minimised from a real Nitro output fixture that still exists in the test suite: test/js/bun/http/js-sink-sourmap-fixture/. The now-deleted reduction even uses the identical file naming (index.mjs + chunks/stream.mjs). The original fixture was not given the same .todo treatment, and with the skip removed it will hang.
Code path
bun-server.test.ts:405-415runsBun.spawnSync({ cmd: [bunExe(), "js-sink-sourmap-fixture/index.mjs"], ... })— synchronous, notimeoutoption.js-sink-sourmap-fixture/index.mjs:5577-5599is module-top-level code: it callsBun.serve(...), thenconst result = await fetch(${server.url}/stream). This top-levelawaitmakesindex.mjsHasTLA; the module suspends here with statusEvaluatingAsync.- The
/streamroute is wired to_lazy_VB431L = () => import('./chunks/stream.mjs')(line 5442/5447) via h3'sdefineLazyEventHandler(line 2146-2171), which doesPromise.resolve(factory()).then(...)on first request. So serving the request awaits theimport()promise. chunks/stream.mjs:6hasimport { e as eventHandler } from '../index.mjs';— a static import back to the still-EvaluatingAsync entry.
Why existing code doesn't prevent it
With the custom skip removed, spec step 11.c applies when stream.mjs's fresh Evaluate() DFS visits index.mjs (status evaluating-async, AsyncEvaluation true): stream.mjs.[[PendingAsyncDependencies]] is incremented to 1 and stream.mjs is appended to index.mjs.[[AsyncParentModules]]. stream.mjs's body is therefore not executed; the import() promise stays pending until index.mjs finishes async evaluation. But index.mjs is suspended at await fetch(...), which can only resolve once the /stream handler responds, which requires stream.mjs to evaluate — circular wait.
Bun.serve's listening socket keeps the event loop alive, so the "unsettled top-level await" exit path does not fire. The child process hangs indefinitely. Even if idleTimeout eventually rejects the fetch (→ catch → process.exit(1)), the test still goes from PASS → FAIL on expect(exitCode).toBe(0).
This wasn't caught locally because, per the robobun comment, only dynamic-import-tla-cycle.test.ts was run with the patched JSC; full Bun CI is still blocked on the WebKit preview tarball publishing.
Step-by-step proof
- Test runner thread enters
Bun.spawnSyncatbun-server.test.ts:406and blocks in native code waiting for the child. - Child evaluates
index.mjs→ startsBun.serve→ reachesawait fetch('/stream')at line 5599 → module status becomesEvaluatingAsync, JS stack unwinds. - Server receives the request →
lazyEventHandlercallsimport('./chunks/stream.mjs'). innerModuleEvaluation(stream.mjs)recurses into../index.mjs→ findsAsyncEvaluation == true→ setsstream.[[PendingAsyncDependencies]] = 1, returns. Back instream.mjs:pendingAsyncDependencies > 0→ body NOT executed.import()returns an unsettled promise.- Lazy handler's
.then(r => r.default ...)never fires → no Response →fetchat 5599 never settles →index.mjsnever finishes →stream.mjsis never released. Server socket keeps the loop alive → no process exit. spawnSyncblocks the parent's JS thread; bun:test's async per-test timer cannot interrupt a thread blocked in a native syscall. The wholebun-server.test.tsfile stalls.
Impact
CI-breaking regression: bun-server.test.ts will hang once a build links the patched JSC. Best case (if something eventually times the connection out) the test goes PASS → FAIL.
Fix
Mark bun-server.test.ts:405 as test.todo alongside the two reduced cases (referencing the same follow-up tracking), or add a timeout to the Bun.spawnSync call and convert the assertion to expect the deadlock until the narrower referrer-aware skip lands. Longer term the fixture could be restructured so chunks/stream.mjs no longer statically re-imports index.mjs.
|
Alternative approach in #30656 / oven-sh/WebKit#230: instead of dropping the re-entrancy skip (which regresses the Nitro self-deadlock tests), narrow it with a fourth discriminator — |
`@lexical/react` 0.44+ (and other packages that dispatch dev/prod via `await import()` in a wrapper module) throws "Cannot access 'HISTORY_MERGE_TAG' before initialization" — a TDZ read of a wrapper's post-`await` `export const` binding — when two siblings of the wrapper are dynamic-imported concurrently from a `Promise.all` at the top. Root cause: Bun-specific skip at `innerModuleEvaluation` step 11.c.v that was added for the `require(esm)` / dynamic-import self-deadlock case (TLA A awaits `import(B)`, B imports A) was indistinguishable from the sibling-race case at its decision point, so it skipped the wait there too and let consumers execute while the wrapper's bindings were still in TDZ. Fixed in oven-sh/WebKit#228 by dropping the custom skip entirely and matching spec / Node behaviour for both shapes. See the WebKit PR for full analysis of why no passive signal at 11.c.v distinguishes the two shapes and why the trade-off favours this direction (the broken pattern is a genuine self-deadlock users need to restructure anyway; the previously-crashing pattern is widely used by Meta's Lexical editor framework). Bumps WebKit to `preview-pr-228-3a85cc44`. ## Tests `test/js/bun/resolve/dynamic-import-tla-cycle.test.ts`: - New test exercises the #30634 pattern (sibling dynamic imports via `Promise.all` sharing a TLA wrapper) — PASS. - Existing tests for same-DFS sibling narrowing (tests 3-5, from #30259) — PASS. - Existing deadlock-avoidance tests for the Nitro-style `await import('./chunk')`-loops-back pattern (tests 1-2) — marked `.todo`. These test a behaviour that will require proper referrer-aware skip (threading the dynamic-import referrer from `moduleLoaderImportModule` through `ModuleLoaderPayload` to the evaluate path) — tracked for follow-up.
robobun
force-pushed
the
farm/cb50e83f/fix-tla-sibling-dynamic-import
branch
from
85a4794 to
4c57af9
Compare
|
Closing — #30656 supersedes this PR with a better approach (narrows the re-entrancy skip instead of dropping it, preserving the Nitro deadlock tests). |
1 participant
Problem
@lexical/react0.44+ (and many packages that dispatch dev/prod entries viaawait import()in a wrapper module) throws a TDZ error when imported concurrently:Minimal repro:
Bun throws
ReferenceError: Cannot access BAR before initializationat consumer2.Root cause
Bun-specific skip at
innerModuleEvaluationstep 11.c.v (invendor/WebKit/Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp) fires based on three conditions —depWasAlreadyEvaluatingAsync,asyncEvaluationOrder < asyncOrderWatermark,pendingAsyncDependencies == 0— that are indistinguishable between:import(B); B statically imports A.Promise.allshare a TLA wrapper. Skipping runs consumers too early → TDZ.No passive signal at the decision point distinguishes the two.
Fix
Drops the custom skip in oven-sh/WebKit#228 and bumps
WEBKIT_VERSIONhere.Coordination
Mirrors the oven-sh/WebKit#215 → #30262 pattern: WebKit PR merges first, then this WEBKIT_VERSION gets updated to the merged SHA (currently pointing at
preview-pr-228-3a85cc44preview build). Bun CI stays red until that preview build publishes its tarball.Tests
test/js/bun/resolve/dynamic-import-tla-cycle.test.ts:sibling dynamic imports in Promise.all wait for a shared TLA wrapper— exercises [1.3.14] ESM TDZ error when importing Lexical React modules that re-export through top-level await #30634 pattern.await import(./chunk)-loops-back pattern) — marked.todopending proper referrer-aware skip.